Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Explicit operation order determined by user definition #379

Merged
merged 7 commits into from
May 11, 2024
Merged

Explicit operation order determined by user definition #379

merged 7 commits into from
May 11, 2024

Conversation

skim0119
Copy link
Collaborator

@skim0119 skim0119 commented May 9, 2024 •
edited
Loading

Explicit operation order

  • User-API would remain the same

Description

Previous implementation has ambiguity in determining the order of operation for different boundary conditions: forcing, contact, friction, connection, etc. For example, contact and friction must be calculated after all other boundary conditions are in place, and strictly contact must be computed before friction since friction depends on contact. In various place in the code, we tried to rearange the order without explicitly letting user know:

PyElastica/elastica/modules/base_system.py

Lines 173 to 182 in 87a2e74

_call_contacts_index = []
for idx, feature in enumerate(self._feature_group_synchronize):
if feature.__name__ == "_call_contacts":
_call_contacts_index.append(idx)
# Move to the _call_contacts to the end of the _feature_group_synchronize list.
for index in _call_contacts_index:
self._feature_group_synchronize.append(
self._feature_group_synchronize.pop(index)
)
,

PyElastica/elastica/modules/forcing.py

Lines 72 to 81 in 87a2e74

# Find if there are any friction plane forcing, if add them to the end of the list,
# since friction planes uses external forces.
friction_plane_index = []
for idx, ext_force_torque in enumerate(self._ext_forces_torques):
if isinstance(ext_force_torque[1], AnisotropicFrictionalPlane):
friction_plane_index.append(idx)
# Move to the friction forces to the end of the external force and torques list.
for index in friction_plane_index:
self._ext_forces_torques.append(self._ext_forces_torques.pop(index))
.
This cause ambiguity on user, especially when user wants to define their own boundary condition.

Solution

As long as we let users to write their own boundary conditions, we cannot anticipate every possible cases of operation dependencies. We are shifting to the policy that user must be aware of the boundary operation order.

This PR removes any "back-end" sorting the operation order. Operation order is now strictly defined by the order of user inserting any boundary condition with .using. This applies to forcing, contact, and connection as of now.

Some remaining TODOs:

  • Add warning messages on packaged contact and friction modules
  • Refactor synchronize operations: "first-in" boundary condition computes first
  • Test
    • Fix existing tests
    • Implement new test for testing operation order
  • Documentation (f0fc544)

@skim0119 skim0119 changed the base branch from update/mypy to update-0.3.3 May 9, 2024 08:04
@skim0119 skim0119 self-assigned this May 9, 2024
@skim0119 skim0119 added enhancement New feature or request prio:high Priority level: high labels May 9, 2024
@skim0119 skim0119 force-pushed the feature/order-operation branch 3 times, most recently from e5fa496 to 00d6caa Compare May 9, 2024 08:16
@skim0119 skim0119 changed the title Fix: Make operation order fully determined by user definition Explicit operation order determined at user definition May 9, 2024
@skim0119 skim0119 changed the title Explicit operation order determined at user definition Explicit operation order determined by user definition May 9, 2024
@skim0119 skim0119 marked this pull request as ready for review May 9, 2024 17:52
armantekinalp
armantekinalp requested changes May 9, 2024
View reviewed changes
Copy link
Contributor

@armantekinalp armantekinalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some questions. But overall looks good.

elastica/modules/feature_group.py Outdated Show resolved Hide resolved
elastica/modules/connections.py Show resolved Hide resolved
elastica/modules/contact.py Show resolved Hide resolved
armantekinalp
armantekinalp approved these changes May 11, 2024
View reviewed changes
Copy link
Contributor

@armantekinalp armantekinalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@skim0119 skim0119 merged commit 9a92a49 into update-0.3.3 May 11, 2024
11 checks passed
@skim0119 skim0119 deleted the feature/order-operation branch May 11, 2024 02:32
@skim0119 skim0119 mentioned this pull request May 11, 2024
Merged
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@armantekinalp armantekinalp armantekinalp approved these changes

@bhosale2 bhosale2 Awaiting requested review from bhosale2 bhosale2 is a code owner

@Ali-7800 Ali-7800 Awaiting requested review from Ali-7800

@sy-cui sy-cui Awaiting requested review from sy-cui

Assignees

@skim0119 skim0119

Labels
enhancement New feature or request prio:high Priority level: high
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@skim0119 @armantekinalp